-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix stroke not being shown on a selection while drawing #1807
Fix stroke not being shown on a selection while drawing #1807
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this. I noticed this fix assumes that isDrawing implies mSelectionTransform.isIdentity(), which isn’t necessarily true, and thereby uncovers another issue:
Steps to reproduce:
- Switch to selection tool and make a selection
- Hold alt and drag any of the corner handles to transform the selection
- Switch to, say, pencil tool and draw across the selection.
Now while drawing across the selection, both the stroke and the selection will be drawn as if the selection was not transformed, but once you stop drawing both will be transformed.
I still have a bunch of other things to worry about unfortunately, so I’ll let you decide whether you want to treat it as separate issues and merge this as is or fix both in one go. It’s also why I still haven’t been able too lok at your other PR…
Hi Jacob, I hope you're doing well. I know that life can get in the way, so don't feel bad about not having reviewed the other PR yet, we'll get there eventually. 😃 You're right about that other bug! I completely forgot about the temporary move state 😅 ... The transformation should actually be in a identity state in cases where you are able to draw on the canvas again, because the "leavingThisTool" function should commit the transformation before going back to another tool. I've pushed a fix for that too now and tested the cases that I could think of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The transformation should actually be in a identity state in cases where you are able to draw on the canvas again
I’ve added an assertion for this assumption to make it more explicit.
@MrStevns @J5lx I've had a chance to look at this. The tool change seem sensible. However, this isn't enough to solve the issue completely. If you create a selection, switch to a drawing tool, and move the selection with the arrow shortcuts, and then draw a stroke, then transform is not identity and isDrawing is true. |
I am not able to reproduce that behavior, are you referring to the 'M' shortcut? Once you switch from the Move tool back to a drawing tool, using a shortcut or not, that should still trigger the LeavingTool function and apply the transformation. edit: .... ah you meant the arrow keys on the keyboard.. 😅 bah yeah i completely forgot about those... selection-bug-investigation-trimmed.mp4 |
I've investigated this and can now say that it's a bug. Moving the selection when the current tool is Select, should not modify the keyframe, and therefore neither the selection transform. The behavior should work as such: I'll address this in my upcoming PR for floating selection. |
How to reproduce:
Expected:
Stroke is drawn
Actual:
Stroke is not drawn until you stop drawing.